feat(clp-json): Add session token support for presigned URL authentication. #680
Conversation
WalkthroughThe pull request introduces support for AWS session tokens across multiple components of the system. The changes modify the AWS authentication mechanism to optionally include a session token during authentication processes. This enhancement allows for more flexible AWS credential management by adding support for temporary security credentials. The modifications span several files in the core and S3 reader components, updating constructors, method signatures, and help text to accommodate the new optional session token parameter. Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthSigner as AWS Authentication Signer
participant S3 as AWS S3
User->>AuthSigner: Create with access key, secret key, optional session token
AuthSigner->>S3: Generate signed URL
alt Session Token Present
AuthSigner->>S3: Include session token in authentication
else No Session Token
AuthSigner->>S3: Authenticate without session token
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/core/src/clp_s/ReaderUtils.cpp (1)
6-6: Remove unused header.The <unordered_map> header appears to be unused in this file.
-#include <unordered_map>components/core/src/clp_s/CommandLineArguments.cpp (1)
277-277: LGTM! Consider improving help text readability.The addition of AWS session token support in the help text is accurate and consistent across all command modes. However, to improve readability, consider breaking the authentication requirements into multiple lines.
Apply this formatting improvement to all three locations:
- " variables, and optionally the AWS_SESSION_TOKEN environment variable." + " variables, and optionally the AWS_SESSION_TOKEN environment" + " variable."Also applies to: 431-431, 585-585
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/src/clp/aws/AwsAuthenticationSigner.cpp(2 hunks)components/core/src/clp/aws/AwsAuthenticationSigner.hpp(3 hunks)components/core/src/clp_s/CommandLineArguments.cpp(3 hunks)components/core/src/clp_s/InputConfig.hpp(1 hunks)components/core/src/clp_s/ReaderUtils.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
components/core/src/clp_s/InputConfig.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/CommandLineArguments.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/aws/AwsAuthenticationSigner.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/aws/AwsAuthenticationSigner.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ReaderUtils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (8)
components/core/src/clp_s/InputConfig.hpp (1)
14-14: LGTM! The AWS session token environment variable constant is well-defined.The constant follows the established naming convention and uses the standard AWS environment variable name.
components/core/src/clp/aws/AwsAuthenticationSigner.hpp (3)
5-5: LGTM! Appropriate header inclusion.The std::optional header is correctly included to support the optional session token parameter.
73-80: LGTM! Well-structured constructor signature.The constructor is properly updated to handle the optional session token parameter, maintaining a clean interface with required parameters first and optional parameters last.
138-138: LGTM! Member variable follows class conventions.The m_session_token member variable follows the class's naming convention with the m_ prefix.
components/core/src/clp_s/ReaderUtils.cpp (2)
170-174: LGTM! Proper session token handling.The code correctly handles the optional session token by:
- Initializing with std::nullopt
- Checking for environment variable presence
- Converting to std::string only when the token exists
176-180: LGTM! Proper signer initialization.The AwsAuthenticationSigner is correctly initialized with all required credentials, including the optional session token.
components/core/src/clp/aws/AwsAuthenticationSigner.cpp (2)
251-258: LGTM! Proper session token parameter handling.The code correctly:
- Creates the session token parameter only when present
- Uses the correct AWS parameter name "X-Amz-Security-Token"
- Properly encodes the token value
Line range hint
260-271: LGTM! Clean string formatting with session token.The canonical query string formatting properly incorporates the optional session token parameter while maintaining readability.
gibber9809
changed the title
| return fmt::format( | ||
| "{}={}&{}={}&{}={}&{}={}&{}={}", | ||
| "{}={}&{}={}&{}={}&{}={}{}&{}={}", |
There was a problem hiding this comment.
What will be the behavior is m_session_token.has_value() == false?
I assume session_token_parameter will have an empty string as value hence the extra {} will evaluate to nothing?
There was a problem hiding this comment.
I personally feel it might be better to append session_token_parameter explicitly, since te number of {}={} already make the original fmt::format pretty hard to decipher (I had to count by myself when making the change), but ulitimately it's up to you.
There was a problem hiding this comment.
Sure. Will refactor to append session token explicitly.
| @@ -12,6 +12,7 @@ constexpr std::string_view cXAmzAlgorithm{"X-Amz-Algorithm"}; | |||
| constexpr std::string_view cXAmzCredential{"X-Amz-Credential"}; | |||
| constexpr std::string_view cXAmzDate{"X-Amz-Date"}; | |||
| constexpr std::string_view cXAmzExpires{"X-Amz-Expires"}; | |||
| constexpr std::string_view cAmzSecurityToken{"X-Amz-Security-Token"}; | |||
There was a problem hiding this comment.
missing an "X"?
| constexpr std::string_view cAmzSecurityToken{"X-Amz-Security-Token"}; | |
| constexpr std::string_view cXAmzSecurityToken{"X-Amz-Security-Token"}; |
|
|
||
| #include <spdlog/spdlog.h> | ||
|
|
||
| #include "../clp/aws/AwsAuthenticationSigner.hpp" | ||
| #include "../clp/CurlDownloadHandler.hpp" |
There was a problem hiding this comment.
Also a leftover. Will delete.
| #include <string_view> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
didn't see this used in the file. Is it intended?
There was a problem hiding this comment.
Nope -- leftover from when I was going to pass the token as an http header. Will delete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/core/src/clp_s/ReaderUtils.cpp (2)
174-178: Consider single-line initialization for better readability.The constructor initialization is correct, but since it's a simple case with three parameters, consider using a single-line format:
- clp::aws::AwsAuthenticationSigner signer{ - aws_access_key, - aws_secret_access_key, - optional_aws_session_token - }; + clp::aws::AwsAuthenticationSigner signer{aws_access_key, aws_secret_access_key, optional_aws_session_token};
168-172: Consider adding specific error logging for session token issues.The current implementation silently handles session token related failures. Consider adding specific error logging when session token validation fails to help with debugging.
if (nullptr != aws_session_token) { optional_aws_session_token = std::string{aws_session_token}; + SPDLOG_DEBUG("Using AWS session token for authentication"); } clp::aws::AwsAuthenticationSigner signer{ aws_access_key, aws_secret_access_key, optional_aws_session_token };Also applies to: 174-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp/aws/AwsAuthenticationSigner.cpp(1 hunks)components/core/src/clp/aws/constants.hpp(1 hunks)components/core/src/clp_s/ReaderUtils.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp/aws/constants.hpp
- components/core/src/clp/aws/AwsAuthenticationSigner.cpp
🧰 Additional context used
📓 Path-based instructions (1)
components/core/src/clp_s/ReaderUtils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (2)
components/core/src/clp_s/ReaderUtils.cpp (2)
4-4: Good addition of explicit dependency.The
<string>header is now properly included for string operations, rather than relying on indirect inclusion through other headers.
168-172: Well-structured session token handling.The implementation properly handles the optional nature of session tokens using std::optional and follows the same pattern as other AWS credential checks.
gibber9809
changed the title
Description
This PR adds support for using an aws session token (the X-Amz-Security-Token request parameter) to authenticate requests against S3 in combination with the access key ID and secret access key.
We add an std::optional<> option to AwsAuthenticationSigner to represent this new optional argument, and conditionally use it to generate the canonical query string.
Inside of clp-s we now look for the
AWS_SESSION_TOKENenvironment variable, and pass it to AwsAuthenticationSigner when it exists.Note that we chose to pass the session token as a url parameter instead of as an http header, because passing the session token as a url parameter is required for some configurations per the docs.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Improvements